Plugfest work and various RDM test fixes and improvements #1067
Conversation
peternewman
changed the title
|
This should finally be green and ready for review @nomis52 . Similar to last time, some of the work was done at the plugfest, so wants more of a sanity check than normal. |
| # Incrementing list, so we can find out which bit we have where in memory | ||
| data = '' | ||
| for i in xrange(0, self.MAX_PDL): | ||
| data += chr(i) |
There was a problem hiding this comment.
This is going to break formatting (consider \n and null in the data).
Can we just use 0123456789 repeating instead?
There was a problem hiding this comment.
It doesn't actually, see below (this also renders the same in the web UI).
Also in the case of the bug I mentioned in the email, 0123456789 repeating wouldn't allow you to see what offset in the data was being misused. I guess we could do a compromise of 012345678911234567892123456789 or something, but I'm not sure if that's very clear, especially if you're looking at a packet dump (although I guess it probably has an ASCII decode too).
./tools/rdm/rdm_responder_test.py -u 99 -t GetMaxPacketSize -s --debug 7a70:ffffff00
OLA Responder Tests Version 0.10.1
Fetching UID list from server
Found UID 7a70:ffffff00
The following devices were detected and will be reconfigured
7a70:ffffff00
7a70:ffffff01
7a70:ffffff02
7a70:ffffff03
7a70:ffffff04
7a70:ffffff05
7a70:ffffff06
Restricting tests to GetMaxPacketSize
Starting tests, universe 99, UID 7a70:ffffff00, broadcast write delay 0ms, inter-test delay 0ms
Test order is [GetMaxPacketSize]
GetMaxPacketSize: Check if the responder can handle a packet of the maximum size.
GET: uid: 7a70:ffffff00, pid: DEVICE_INFO (0x0060), sub device: 0, data: '\x00\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\x7f\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f\xa0\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf\xc0\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf\xe0\xe1\xe2\xe3\xe4\xe5\xe6'
Response: RDMResponse(type=NACK, reason="Format Error"), PID: 0x0060, TN: 0
GetMaxPacketSize: Passed
------------------- Summary --------------------
Test Run: 2016-04-30 06:45:45 PM
UID: 7a70:ffffff00
------------------- Warnings --------------------
------------------ Advisories -------------------
------------------ By Category ------------------
Error Conditions: 1 / 1 100%
-------------------------------------------------
1 / 1 tests run, 1 passed, 0 failed, 0 broken
There was a problem hiding this comment.
Ah ,yes I forgot that we escape it.
tools/rdm/TestDefinitions.py
Outdated
| self.SendRawSet(ROOT_DEVICE, self.pid) | ||
| # Unknown PID shouldn't be valid for a mandatory PID | ||
| self.AddExpectedResults( | ||
| self.NackSetResult(RDMNack.NR_UNSUPPORTED_COMMAND_CLASS) |
There was a problem hiding this comment.
The standard doesn't (yet) say anything about the order of validation, so unknown pid is valid here
Imagine the implementation looks up first by CC and then PID, rather than PID then CC.
There was a problem hiding this comment.
Can we compromise on a warning or advisory? I appreciate what you're saying, but you may as well only have one NACK code if you're going to ignore obvious precedence/appropriateness. Some similar stuff came up at the Plugfest too.. Aside from the word message being a poor choice, the standard says "The responder cannot comply with request because the message is not implemented in responder.". Assuming message=PID, it's not the right choice of NACK code on a mandatory PID. Also it would be less confusing as a controller writer if they use the more appropriate NACK case.
There was a problem hiding this comment.
Changed to an advisory @nomis52 . Hopefully you're happy with this compromise?
|
Just a handful of comments |
|
Are you happy with my implemented compromise of an advisory rather than a fail for an inappropriate NAck code @nomis52 ? Can we merge this in? |
…nto libusb Conflicts: tools/rdm/TestDefinitions.py
|
This is now in sync and has fewer changes than initially. |
|
Can we merge this too @nomis52 ? |
|
LGTM |
2 participants
No description provided.